It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

&impl ContainsEntity

I'm reluctant to do this as it will break object safety. That will be a pretty widespread problem when trying to fix this though.

I'm reluctant to do this as it will break object safety. That will be a pretty widespread problem when trying to fix this though.

It won't be needed with @cart 's suggestion. I'll revise the CL.

How do we feel about immutable entity events though?

How do we feel about immutable entity events though?

Generally open to it, but haven't thought about this hard enough to be convinced that they're the only solution :)

I've updated the PR with the simplified implementation. Great idea! :)

Generally open to it, but haven't thought about this hard enough to be convinced that they're the only solution :)

I'm open to suggestions!

From my usage of the engine so far, I have yet to find a use case for propagating events, which as I understand, is the only reason entity events are mutable.

So from my perspective, I could easily envision all entity events being immutable by default, and only mutable if #[entity_event(propagate)] is specified. Attempting to propagate an event not flagged with #[entity_event(propagate)] seems like a degenerate case to me, and so we could easily treat it as an error.

If we do that, we could then implement set_event_target with unreachable!() for immutable events, since it would never be called. Additionally, I would go as far as marking set_event_target as unsafe. In my mind, this function should never be called externally.

On the topic of immutable entity events, I'm on board for that, but I think we'd do that at the trait level and break it out as SetEntityEventTarget (where specifying propagate in the EntityEvent derive would provide that impl).

From my usage of the engine so far, I have yet to find a use case for propagating events, which as I understand, is the only reason entity events are mutable.

Making them mutable would also theoretically enable a bulk API (ex: commands.trigger_targets(MyEvent { entity: Entity::PLACEHOLDER }, [e1, e2, e3])). But that feels a bit dirty to me / I'm pretty fine with losing it.

I've added SetEntityEventTarget as discussed. Much cleaner implementation than what I had in mind! :)

For me, the only remaining issue is the EntityCommand::trigger call.

Should I split that into a separate PR or should I continue working in this PR?

Any opinions on how to refactor trigger?

In my opinion, the current implementation goes against Rust nomenclature. Typically in Rust we have functions like verb which take some data as input, or verb_with which take a constructor Fn.
We only have trigger(f: impl FnOnce(Entity) -> E).

To fix this properly, I think trigger should just be trigger(e: E) and we add trigger_with(f: impl FnOnce(Entity) -> E); but that would be a breaking change (which this PR already is? Maybe? 😅). I'm also open to other ideas on the naming.